Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add s3 connector TCTC-6566 #1273

Merged
merged 5 commits into from
Sep 13, 2023
Merged

feat: add s3 connector TCTC-6566 #1273

merged 5 commits into from
Sep 13, 2023

Conversation

andreamouraud
Copy link
Contributor

@andreamouraud andreamouraud commented Sep 11, 2023 •
edited
Loading

Change Summary

Add a S3 connector using the AWS Security Token Service API

Set up

The following properties need to be injected into the connector by the backend:

  • sts_access_key_id
  • sts_secret_access_key
  • workspace_id

The external_id field is read only and represents the injected workspace_id, it is used to create the corresponding AWS role

Retrieving data

The file specified in the S3DataSource is loaded from the S3 bucket using the injected credentials, https://github.com/ToucanToco/peakina is then used to load the DataFrame from the file

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable

@andreamouraud andreamouraud added enhancement New feature or request wip labels Sep 11, 2023
@andreamouraud andreamouraud self-assigned this Sep 11, 2023
Sanix-Darker
Sanix-Darker approved these changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@Sanix-Darker Sanix-Darker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this amazing work, LGTM 🔥
Just a simple nit + maybe the image is too big in term of size idk... (not a blocking point)?

tests/s3/test_s3.py Outdated Show resolved Hide resolved
@Sanix-Darker
Copy link
Contributor

Sanix-Darker commented Sep 12, 2023 •
edited
Loading

You can also update the CHANGELOG.md by adding new entry about the new connector.

EDIT: on this PR or the one for the bumping, as you wish !

@andreamouraud
Copy link
Contributor Author

@Sanix-Darker great idea, will do in the bump 👍

@andreamouraud andreamouraud merged commit 09b0203 into master Sep 13, 2023
@andreamouraud andreamouraud deleted the f/s3 branch September 13, 2023 10:06
@andreamouraud andreamouraud mentioned this pull request Sep 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sanix-Darker Sanix-Darker Sanix-Darker approved these changes

Assignees

@andreamouraud andreamouraud

Labels
enhancement New feature or request Need Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andreamouraud @Sanix-Darker